-
Notifications
You must be signed in to change notification settings - Fork 370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DA1469x: Clock time shifts on reboots #3321
Conversation
9367986
to
d85d3fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number of functions are moved around that makes it harder to see what is new and what was really modified/added.
d85d3fc
to
0d38224
Compare
I see your concern; I separated the original commit into two, one only rearranges the functions with no changes whatsoever, and the second one adds the missing services to the API. |
If your first commit does not change functionality is it really needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't move code around in .c
files - this breaks git annotate
Yeah, keeping the git annotate may indeed outweigh the benefits of having the functions in a neat order. To that extent, should I keep the "da1469x/clock: Eliminate magic numbers" commit, or should I leave those lines unchanged as well? |
Keep it, removing magic numbers is fine. |
0d38224
to
561a02b
Compare
Addressed all current reviewer comments. |
561a02b
to
ae37693
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it. Seems to be working great. Once all the comments are addressed, we can merge it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one issue left, otherwise lgtm.
please also fix coding style to pass CI
yeap, looks ok after this typo and minor coding style is fixed:) |
ae37693
to
781489b
Compare
This commit adds the missing services to the driver API. It also makes the actual calibration of the XTAL32K clock a syscfg option, defaulting to 0.
This commit replaces the magic numbers with meaningfully named constants.
This commit streamlines the LP clock API, by offering clean access to the LP clock selected via sysconfig.
This commit adds the function that calculates the RTC divider registers, and invokes it when the LP clock frequency changes.
781489b
to
c4e9455
Compare
If the LP clock's frequency is not an integer multiple of the specified OS tick frequency, the period of OS ticks will be incorrect due to an integer division truncation error (to the tune of almost 1% for RCX), resulting in OS Time gradually creeping away from the actual time. This commit fixes the issue, by maintaining the fractional OS tick value and advance OS Time accordingly.
This commit adds masking to register bitfields, where a wrong syscfg value may result in the inadvertent overrides of neighboring bitfields.
c4e9455
to
f0cc564
Compare
If the LP clock's frequency is not an integer multiple of the specified OS tick frequency, the period of OS ticks will be incorrect due to integer division truncation errors (to the tune of almost 1% for RCX), resulting in OS Time
gradually creeping away from the actual time. This PR fixes the issue, by maintaining the fractional OS tick value and advance OS Time accordingly.
This PR also improves on the implementation of the DA1469x clock driver and updates the RTC divider registers after the selected LP clock is calibrated.